Make dumping work with the `irM.instN` tagged InstId format #6168

danakj · 2025-10-06T19:12:49Z

As of #5997, InstId now contains a tag of the CheckIRId inside it, which works fine for debugging when dumping a C++ variable, but did not work anymore when trying to dump other InstIds that had been dumped. Teach lldb's dump command to pull apart the ir1.inst2 format and change the MakeInstId methods in dump.cpp to take the CheckIRId as an input. Since you could now possibly dump inst ids from another IR, provide an overload in Check:: that allows for this. And CHECK in SemIR:: if you try to dump something from the wrong IR.

Now this works again:

(lldb) dump context ir1.inst21
ir1.inst21: {kind: ClassType, arg0: class0, arg1: specific<none>, type: type(TypeType)}
  - type: type(TypeType): type; {kind: TypeType, type: type(TypeType)}
  - value: concrete_constant(ir1.inst21)
  - loc: LocId(<none>)

jonmeow · 2025-10-06T19:45:42Z

Just to note, on this PR, CheckIRId uses "check_ir" and ImportIRId uses "ir". It sounds like you're saying InstId/IdTag is using "ir" for CheckIRId; had you considered aligning the prefix as part of adding a dependency on which it is?

danakj · 2025-10-06T20:15:44Z

Just to note, on this PR, CheckIRId uses "check_ir" and ImportIRId uses "ir". It sounds like you're saying InstId/IdTag is using "ir" for CheckIRId; had you considered aligning the prefix as part of adding a dependency on which it is?

I have used check_ir_id in the variable names I am introducing. Are you suggesting check_ir1.inst2 as the dump format? That feels like it's getting long to me...

That said, I would be just fine with that if we omitted the ir from the id printing when it's from the current IR. I do want to do that, but it requires us to plumb File into Print, which I think we need to have a discussion about separate from just making dumping work again.

jonmeow · 2025-10-06T20:23:13Z

Yes, that's right. The incorrect prefix seemed to be part of why you were confused here, and it confused me too, so I'd thought maybe it'd be worth the characters.

danakj · 2025-10-06T21:08:44Z

Yes, that's right. The incorrect prefix seemed to be part of why you were confused here, and it confused me too, so I'd thought maybe it'd be worth the characters.

I see, yeah the biggest confusion was that I thought it must be referring to a foreign IR if it was appearing in the id like that. But the id number didn't match the foreign file. I did not at all consider it could be referring to the current file.

jonmeow

To be sure, I have very strong concerns about the multiple different uses of "ir". Look for example at the YAML format in testdata -- line 91 and line 124 use "ir1" with different meaning. This seems like it'll be a significant debugging hindrance; I'm maybe too familiar with what the prefixes mean, but also do you expect people joining to be able to understand this?

Also, taking a step back to one thing you said earlier about "check_ir" feeling "long". Is the main use of this copy-paste from other debug output, or are you expecting you'll usually type it in from scratch?

Speaking for myself, I see three possible answers here:

Change IdTag to match CheckIRId
- ImportIRId -> "ir", CheckIRId -> "check_ir", IdTag -> "check_ir"
Change ImportIRId and IdTag to always be explicit
- ImportIRId -> "import_ir", CheckIRId -> "check_ir", IdTag -> "check_ir"
Swap ImportIRId and CheckIRId
- ImportIRId -> "import_ir", CheckIRId -> "ir", IdTag -> "ir"

Personally, I'd favor (2) actually -- we should probably always clarify because confusion isn't worth the shorter name. (I could mainly see (3) if typing this out is a common use-case) But, partly noting this if it's worth further discussion (I assume this wasn't discussed in detail since the review was reassigned to me).

jonmeow · 2025-10-06T21:44:37Z

scripts/lldbinit.py

-    if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
-        if m[1] in id_types:
+    # Look for <irN>.<type><id> as a single argument.
+    if m := re.fullmatch("ir(\\d+)\\.([a-z_]+)(\\d+)", args[1]):


Had you considered making this optional (e.g.. (ir(\\d+)\\.)?) to make this a single regex?

The arguments to the MakeInstId function are different depending on if there's an ir or not. I guess I could look for the group being present and branch, but this feels at least as straightforward to me.

toolchain/check/dump.cpp

scripts/lldbinit.py

jonmeow · 2025-10-06T22:28:11Z

scripts/lldbinit.py

          a Parse::Context, or a Lex::TokenizeBuffer.
  EXPR is a C++ expression such as a variable name. Use `--` to prevent it from
       being treated as a TYPE and ID.
+  IR is the CheckIRId(N) in the form `irN`.


Should this mention that it only supports inst? That seems new, and doesn't appear to be otherwise documented.

That will become wrong in the future, so I don't feel that we should. Maybe I can say something about what the dumped ids look like instead.

Perhaps still document that it's a subset, and where to look for the supported list? Or change the approach, and just drop the IR on the floor when it's not used?

Right now reading TYPE documentation says everything is allowed, so as long as it's a subset I think it's worth clarifying that the disagreement between TYPE documentation and behavior.

Yep, I have added some clarification and mentioned InstId without saying it's the only one that will be like this.

scripts/lldbinit.py

toolchain/sem_ir/dump.cpp

danakj · 2025-10-07T14:21:23Z

To be sure, I have very strong concerns about the multiple different uses of "ir". Look for example at the YAML format in testdata -- line 91 and line 124 use "ir1" with different meaning. This seems like it'll be a significant debugging hindrance; I'm maybe too familiar with what the prefixes mean, but also do you expect people joining to be able to understand this?

Also, taking a step back to one thing you said earlier about "check_ir" feeling "long". Is the main use of this copy-paste from other debug output, or are you expecting you'll usually type it in from scratch?

Speaking for myself, I see three possible answers here:

Change IdTag to match CheckIRId

ImportIRId -> "ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Change ImportIRId and IdTag to always be explicit

ImportIRId -> "import_ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Swap ImportIRId and CheckIRId

ImportIRId -> "import_ir", CheckIRId -> "ir", IdTag -> "ir"

Personally, I'd favor (2) actually -- we should probably always clarify because confusion isn't worth the shorter name. (I could mainly see (3) if typing this out is a common use-case) But, partly noting this if it's worth further discussion (I assume this wasn't discussed in detail since the review was reassigned to me).

I thought about 3 as well after I left work last night. I think it might be the best option, because yeah sometimes you do have to type things - I tended to type inst31 or whatever a lot before. Which is kind of why I want to get rid of the irN prefix for the current file too.

If possible I would like to separate all of these concerns from getting debugging/dumping functioning again though. Right now, we are printing inst ids that we can not dump, because constructing an inst id in the debugger does so without a tag, and the functions have no way to provide the ir id for the tag.

Will address the other feedback as well, and will reassign this review to you (thanks for looking at it)

danakj

PTAL

scripts/lldbinit.py

danakj · 2025-10-07T14:23:12Z

scripts/lldbinit.py

          a Parse::Context, or a Lex::TokenizeBuffer.
  EXPR is a C++ expression such as a variable name. Use `--` to prevent it from
       being treated as a TYPE and ID.
+  IR is the CheckIRId(N) in the form `irN`.


That will become wrong in the future, so I don't feel that we should. Maybe I can say something about what the dumped ids look like instead.

scripts/lldbinit.py

danakj · 2025-10-07T14:25:24Z

scripts/lldbinit.py

-    if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
-        if m[1] in id_types:
+    # Look for <irN>.<type><id> as a single argument.
+    if m := re.fullmatch("ir(\\d+)\\.([a-z_]+)(\\d+)", args[1]):


The arguments to the MakeInstId function are different depending on if there's an ir or not. I guess I could look for the group being present and branch, but this feels at least as straightforward to me.

scripts/lldbinit.py

toolchain/check/dump.cpp

danakj · 2025-10-07T14:46:04Z

To be sure, I have very strong concerns about the multiple different uses of "ir". Look for example at the YAML format in testdata -- line 91 and line 124 use "ir1" with different meaning. This seems like it'll be a significant debugging hindrance; I'm maybe too familiar with what the prefixes mean, but also do you expect people joining to be able to understand this?
Also, taking a step back to one thing you said earlier about "check_ir" feeling "long". Is the main use of this copy-paste from other debug output, or are you expecting you'll usually type it in from scratch?
Speaking for myself, I see three possible answers here:

Change IdTag to match CheckIRId

ImportIRId -> "ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Change ImportIRId and IdTag to always be explicit

ImportIRId -> "import_ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Swap ImportIRId and CheckIRId

ImportIRId -> "import_ir", CheckIRId -> "ir", IdTag -> "ir"

Personally, I'd favor (2) actually -- we should probably always clarify because confusion isn't worth the shorter name. (I could mainly see (3) if typing this out is a common use-case) But, partly noting this if it's worth further discussion (I assume this wasn't discussed in detail since the review was reassigned to me).

I thought about 3 as well after I left work last night. I think it might be the best option, because yeah sometimes you do have to type things - I tended to type inst31 or whatever a lot before. Which is kind of why I want to get rid of the irN prefix for the current file too.

If possible I would like to separate all of these concerns from getting debugging/dumping functioning again though. Right now, we are printing inst ids that we can not dump, because constructing an inst id in the debugger does so without a tag, and the functions have no way to provide the ir id for the tag.

Will address the other feedback as well, and will reassign this review to you (thanks for looking at it)

I have done the third one in #6172

danakj requested a review from a team as a code owner October 6, 2025 19:12

danakj requested review from josh11b and removed request for a team October 6, 2025 19:12

github-actions bot added infrastructure toolchain labels Oct 6, 2025

debug-irs

567d7a9

danakj force-pushed the debug-irs branch from 09f60b6 to 567d7a9 Compare October 6, 2025 19:33

josh11b requested review from jonmeow and removed request for josh11b October 6, 2025 20:24

jonmeow reviewed Oct 6, 2025

View reviewed changes

toolchain/sem_ir/dump.cpp Outdated Show resolved Hide resolved

review

5b5b7b6

danakj commented Oct 7, 2025

View reviewed changes

danakj requested a review from jonmeow October 7, 2025 14:31

danakj mentioned this pull request Oct 7, 2025

Dump CheckIRId more explicitly for InstId tags #6172

Closed

wording

d3d29f9

danakj closed this Oct 7, 2025

danakj deleted the debug-irs branch October 7, 2025 22:01

Make dumping work with the irM.instN tagged InstId format #6168

Make dumping work with the irM.instN tagged InstId format #6168

Conversation

danakj commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonmeow commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danakj commented Oct 6, 2025

Uh oh!

jonmeow commented Oct 6, 2025

Uh oh!

danakj commented Oct 6, 2025

Uh oh!

jonmeow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danakj commented Oct 7, 2025

Uh oh!

danakj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danakj commented Oct 7, 2025

Uh oh!

Uh oh!

Make dumping work with the `irM.instN` tagged InstId format #6168

Make dumping work with the `irM.instN` tagged InstId format #6168

danakj commented Oct 6, 2025 •

edited

Loading

jonmeow commented Oct 6, 2025 •

edited

Loading

jonmeow left a comment •

edited

Loading